Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add podcast support to WorkPageButtons component #68

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

ThomasGross
Copy link
Contributor

Copy link
Contributor

@Adamik10 Adamik10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got one consideration for ya 😉 Also ask me if you need any info to fix the merge conflicts :))

@@ -56,6 +55,20 @@ const WorkPageButtons = ({ workId }: WorkPageButtonsProps) => {
</Button>
</>
)}
{isPodcast(selectedManifestation) && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use the same button that we use for AudioBook here too? And then just switch the material type in the ariaLabel and the inner Button text depending on isAudioBook and isPodcast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like having them separated. This is because at some point they might introduce another material type or condition to consider. Also it is more readable in this way IMO. But I understand your point, I was a bit back and forward on this one.

@ThomasGross ThomasGross merged commit 37b5c46 into main Dec 9, 2024
9 checks passed
@ThomasGross ThomasGross deleted the DDFBRA-245-bruger-skal-kunne-prove-podcast branch December 9, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants